Object literals with computed property names should allocate fixed slots
Categories
(Core :: JavaScript Engine, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox137 | --- | fixed |
People
(Reporter: iain, Assigned: abdoatef.ab, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
An object literal like {x:1, y: 2}
generates a NewObject
op that allocates an object of the required size and shape, then fills it in using InitProp
. However, if it has a computed property key, like {x: 1, [key]: 2}
, then we don't know the final shape. In that case, we fall back to emitting NewInit, which will call into NewPlainObject and allocate an object with AllocKind::Object0. We will allocate an object with no fixed slots, and then immediately allocate dynamic slots to hold the computed property.
We can't be completely precise here (consider let key = "x"; let o = { x: 1, [key]: 2}
), but redefined properties should be rare enough that allocating a slot per defined property will not waste meaningful amounts of memory.
One possible implementation would be to add an operand to JSOp::NewInit indicating the expected number of properties, and reading it in NewObjectOperation. The NewObject baseline ICs work by calling NewObjectOperation and using the result as a template for future allocations, so if we can return a better sized object from NewObjectOperation, we shouldn't need to do any additional work in the jits.
This will make property access faster and reduce the likelihood that we allocate dynamic slots for these objects.
I'm not aware of this path being particularly hot in any benchmarks, but V8 did optimize it a while back.
Assignee | ||
Comment 1•5 months ago
|
||
I would like to work on this bug. Are there any resources available to help me get started with it?
Comment 2•5 months ago
|
||
So, first and foremost make sure you can build and test SpiderMonkey.
The next thing to do would be to rummage through the parser and start to develop an understanding of the parse tree that gets generated. when you build the shell you get access to the parse
builtin function which will dump the AST such that you can start to see how it looks:
js> parse("let x = {[12]: 12}")
(StatementList [(LetDecl [(AssignExpr x
(ObjectExpr [(PropertyDefinition (ComputedName 12)
12)]))])])js>
You'll also want to get familiar with bytecode and how to read and understand it. the dis
function will help here:
js> function f() { let x = {[12]: 12} }
js> dis(f)
{
"file": "typein",
"lineno": 2,
"column": 11,
"immutableFlags": [
"IsFunction",
"HasMappedArgsObj"
],
"functionName": "f",
"functionFlags": [
"NORMAL_KIND",
"BASESCRIPT",
"CONSTRUCTOR"
]
}
loc line op
----- ---- --
main:
00000: 2 Uninitialized # UNINITIALIZED
00001: 2 InitLexical 0 # UNINITIALIZED
00005: 2 Pop #
00006: 2 NewInit # OBJ
00007: 2 Int8 12 # OBJ 12
00009: 2 ToPropertyKey # OBJ TOPROPERTYKEY(12)
00010: 2 Int8 12 # OBJ TOPROPERTYKEY(12) 12
00012: 2 InitElem # OBJ
00013: 2 InitLexical 0 # OBJ
00017: 2 Pop #
00018: 2 DebugLeaveLexicalEnv #
00019: 2 RetRval #
Source notes:
ofs line column pc delta desc args
---- ---- ------ ----- ------ ---------------- ------
0: 2 11 0 [ 0] colspan colspan 3
2: 2 14 6 [ 6] colspan colspan 11
4: 2 25 6 [ 0] breakpoint-step-sep
5: 2 25 7 [ 1] colspan colspan 1
7: 2 26 19 [ 12] colspan colspan 10
9: 2 36 19 [ 0] breakpoint
Exception table:
kind stack start end
Scope notes:
index parent start end
1 (none) 0 19
GC things:
index type value
0 Scope function {} -> global
1 Scope function lexical {
0: let x (frame slot 0)
} -> function -> global
Next, we'll want to figure out how to estimate the computed property count. My intuition says something along the lines of what was done in Bug 1823811. In this case, when starting to parse an ObjectExpr
we'll default the computed property count estimate to 0; then, when we encounter a nested ComputedName, we will need to update the count for the enclosing ObjectExpr.
After that, as Iain suggested, we'll want a parameter for NewInit that suggests an object size class.
Some other pointers;
Reporter | ||
Comment 3•5 months ago
|
||
In this case, when starting to parse an ObjectExpr we'll default the computed property count estimate to 0; then, when we encounter a nested ComputedName, we will need to update the count for the enclosing ObjectExpr.
Note that we probably want to count all properties, not just computed properties. If we have {x: 1, y: 2, [z]: 3}
, then we want to make sure we have enough room for at least 3 properties.
Assignee | ||
Comment 4•4 months ago
|
||
In this case, when starting to parse an ObjectExpr we'll default the computed property count estimate to 0; then, when we encounter a nested ComputedName, we will need to update the count for the enclosing ObjectExpr.
I followed your advice, but I noticed that EmitObject
already has a propertyCount
. Why can't I just use this propertyCount
and emit it using emit2(propCount, JSOp::NewInt)
? Then, I could add a propCount
parameter to NewPlainObject
and allocate the correct number of slots upfront.
Also, shouldn't the allocated slots have different sizes? Still, I don't know how I should proceed with this.
Comment 5•4 months ago
|
||
Huh! fascinating.
Well, I would do a little validation that said property count is approximately correct, but after that, yes roughly your plan looks good (I think your call to emit2 seems backwards)
The allocated object will have a different count of slots depending on the estimate of the number of properties it will have. All slots are the same size (sizeof(Value)
) .
You'll need to tweak the definition of NewInit and any consumers therein (ie. [baseline compiler,](https://searchfox.org/mozilla-central/source/js/src/jit/BaselineCodeGen.cpp#2980-2983, warp compiler etc). -- both of those look like they've backed by the NewObject inline cache, here. These seems to use a 'template object', obtained here which would be where you consume your estimate, in Baseline.
Warp looks a bit different, and I haven't traced fully.
My recommendation would be to write a patch for baseline, then tackle warp as a second patch.
Assignee | ||
Comment 6•4 months ago
|
||
Updated•4 months ago
|
Assignee | ||
Comment 7•4 months ago
|
||
i just wanted to check if i am going in the right direction
Comment 8•4 months ago
|
||
Yep! Left review comments, but you're on the right road.
Comment 10•3 months ago
|
||
bugherder |
Description
•